geotiff: drop dead bytearray allocation in uncompressed tiled writer (#1736)#1746
Conversation
There was a problem hiding this comment.
Pull request overview
This PR removes an unused large pre-allocation in the GeoTIFF CPU writer’s uncompressed tiled path, preventing unnecessary peak-memory growth during writes. It also adds a regression test module that exercises uncompressed tiled round-trips across several tile/shape configurations.
Changes:
- Remove the dead
bytearray(n_tiles * tile_bytes)allocation (and associatedmemoryview) in the uncompressed tiled writer path. - Add regression tests covering uncompressed tiled round-trips for exact-division tiles, padded edge tiles, and multiband data.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
xrspatial/geotiff/_writer.py |
Deletes the unused uncompressed-tile pre-allocation and replaces it with explanatory comments. |
xrspatial/geotiff/tests/test_writer_uncompressed_tiled_no_dead_alloc_1736.py |
Adds regression round-trip coverage for uncompressed tiled writes (including edge padding and multiband). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| The uncompressed tiled-write branch of ``_compress_tiles`` previously | ||
| allocated a contiguous ``bytearray`` plus a memoryview ``(n_tiles * | ||
| tw * th * bytes_per_sample * samples)`` bytes long at the top of the | ||
| loop and never read either back. Tile bytes were still built via | ||
| ``tile_arr.tobytes()`` and appended to a list. The dead buffer roughly | ||
| doubled peak memory for an uncompressed write. |
There was a problem hiding this comment.
Fixed in 29ebb71. The module docstring now points at xrspatial.geotiff._writer._write_tiled instead of the non-existent _compress_tiles.
| def test_uncompressed_tiled_round_trip_exact(tmp_path): | ||
| rng = np.random.RandomState(20260512) | ||
| h, w = 96, 144 | ||
| data = rng.randint(0, 200, size=(h, w)).astype(np.uint8) | ||
| da = xr.DataArray(data, dims=['y', 'x']) |
There was a problem hiding this comment.
Fixed in 29ebb71 by adding the explicit allocation check, since the test file name promises one.
Two new tests (test_uncompressed_tiled_peak_memory_single_band and test_uncompressed_tiled_peak_memory_multiband) snapshot tracemalloc peak across a direct call to _write_tiled on the uncompressed branch and assert peak stays under 1.5x raster bytes. Calibration:
| variant | current peak | with dead bytearray restored |
|---|---|---|
| 1024x1024 uint8, tile_size=256 | 1.06x | 2.07x |
| 1024x1024x3 uint8, tile_size=256 | 1.06x | 2.06x |
1.5x leaves room for unrelated implementation drift but firmly catches the regression. I verified locally by monkey-patching _write_tiled to reintroduce the bytearray(n_tiles * tile_bytes) allocation; both new tests fail on the patched version with ratio ~2.06.
…1736) The uncompressed branch of _compress_tiles allocated total_buf = bytearray(n_tiles * tile_bytes) mv = memoryview(total_buf) at the top of the loop and never read either back. Tiles were still built via tile_arr.tobytes() and appended to a list. The buffer was roughly the size of the full uncompressed raster, so a marginal write would OOM while holding both the dead buffer and the tile list. Remove the dead allocation. Existing tile-build logic stays the same; the round-trip is byte-for-byte unchanged. Adds regression coverage in test_writer_uncompressed_tiled_no_dead_alloc_1736.py for the exact divisor, edge-tile padding, and multi-band paths.
Addresses two Copilot review comments on PR #1746: * Update the module docstring's stale reference to ``_compress_tiles`` (which doesn't exist) to point at the actual function name, ``xrspatial.geotiff._writer._write_tiled``. * Add real teeth behind the "no dead alloc" test-module name. The three round-trip tests only covered correctness; nothing in the file would have failed if the ``bytearray(n_tiles * tile_bytes)`` allocation were quietly restored. Two new tests snapshot ``tracemalloc`` peak across a direct ``_write_tiled`` call (uncompressed branch, single- and 3-band 1024x1024 uint8 inputs) and assert peak stays under 1.5x raster bytes. The current implementation lands at ~1.06-1.12x; the deleted bytearray pushed peak to ~2.07x. The 1.5x cap reliably catches reintroduction without flagging unrelated implementation drift. The tracemalloc approach was preferred over renaming the tests/file to "round-trip-only" because it preserves both the original intent of the test name and gives future maintainers an automatic alarm if the regression is reintroduced.
Summary
_compress_tilesuncompressed branch allocatedtotal_buf = bytearray(n_tiles * tile_bytes)and a memoryview, then built every tile viatile_arr.tobytes()and never read the buffer back.Closes #1736.
Test plan
test_writer_uncompressed_tiled_no_dead_alloc_1736.pypins three uncompressed-tiled round-trips: exact-divisor tile size, edge-tile padding, and multi-band.